Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Mark allowArbitraryExtensions as affectsProgramStructure #52437

Merged
merged 5 commits into from
Jan 26, 2023
Merged

Mark allowArbitraryExtensions as affectsProgramStructure #52437

merged 5 commits into from
Jan 26, 2023

Conversation

jakebailey
Copy link
Member

Per #51435 (comment) and skimming the code, this flag only controls whether or not certain diagnostics are shown (see getResolutionDiagnostic).

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jan 26, 2023
@jakebailey jakebailey mentioned this pull request Jan 26, 2023
Merged
sheetalkamat
sheetalkamat requested changes Jan 26, 2023
View reviewed changes
andrewbranch
andrewbranch previously approved these changes Jan 26, 2023
View reviewed changes
@jakebailey jakebailey force-pushed the fix-allowArbitraryExtensions-flag branch from 116ee73 to 225d158 Compare January 26, 2023 20:35
@jakebailey
Copy link
Member Author

Excuse the force push; I wanted to rewrite the PR into the two commits so the actual changed showed the baseline change. Let me know if this looks good.

@jakebailey
Copy link
Member Author

I tacked on an extra commit which sets affectsProgramStructure instead; is that the right thing? Maybe the diff provides the answer? (I am not personally very certain)

@jakebailey
Copy link
Member Author

I'm a little confused about how the baseline says that semantic diagnostics aren't updated now that I've set affectsProgramStructure instead; should both affectsProgramStructure and affectsSemanticDiagnostics be set?

@jakebailey jakebailey changed the title Mark allowArbitraryExtensions as affectsSemanticDiagnostics instead of affectsModuleResolution Mark allowArbitraryExtensions as affectsProgramStructure Jan 26, 2023
@jakebailey
Copy link
Member Author

Just to double check, I don't need affectsSemanticDiagnostics, correct? I ran the test (now that it's correct) and the only change when this option was added was that /lib.d.ts had its diagnostics refreshed (which doesn't seem to matter?)

weswigham
weswigham approved these changes Jan 26, 2023
View reviewed changes
Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably the right level of invalidation, since while it does affect resolution diagnostics, those diagnostics in turn control which files are actually loaded into the program (since resolution results with a resolution diagnostic don't actually get traversed for further imports, iirc). So any resolutions are safe to keep between flag settings, but we may still need to refresh the program structure to find more resolutions.

@jakebailey jakebailey merged commit d63d081 into microsoft:main Jan 26, 2023
@jakebailey jakebailey deleted the fix-allowArbitraryExtensions-flag branch January 26, 2023 21:40
@sheetalkamat
Copy link
Member

/lib.d.ts had its diagnostics refreshed (which doesn't seem to matter?)

Thats because you have unknowingly made the resolvedModule not module which means it adds to global scope so any addition or removal will refresh diagnostics for program.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewbranch andrewbranch andrewbranch left review comments

@weswigham weswigham weswigham approved these changes

@sheetalkamat sheetalkamat sheetalkamat approved these changes

Assignees

@jakebailey jakebailey

Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jakebailey @sheetalkamat @weswigham @andrewbranch @typescript-bot